Conversation
Signed-off-by: Helio Chissini de Castro <helio.chissini.de.castro@cariad.technology>
Signed-off-by: Helio Chissini de Castro <helio.chissini.de.castro@cariad.technology>
There was a problem hiding this comment.
Pull request overview
Adds repository configuration support for snippet choices (provenance + per-snippet decisions) and introduces accompanying fixtures/tests, along with project housekeeping updates (version bump and git hook config migration).
Changes:
- Model repo-config
snippet_choicesasSnippetChoicesentries (provenance + list of snippet choices) and add enum parsing for snippet choice reasons. - Add a new repo-config fixture (
tests/data/repo_config/curations.yml) and a dedicated test module for validating/parsing it. - Migrate git hook config from
.pre-commit-config.yamltoprek.toml, and bump package version to0.6.6.
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/ort/models/config/repository_configuration.py |
Switch snippet_choices field to the new structured SnippetChoices model list. |
src/ort/models/config/snippet_choices.py |
Introduces SnippetChoices (provenance + choices list). |
src/ort/models/config/snippet/snippet_provenance.py |
Renames the config model class to SnippetProvenance. |
src/ort/models/config/snippet/snippet_choice.py |
Adds enum conversion for reason values (YAML string → enum). |
tests/data/repo_config/curations.yml |
Adds a repo-config example including snippet_choices and curations/excludes. |
tests/test_repo_config_curations.py |
Adds tests to validate parsing/structure of curations.yml. |
src/ort/models/provenance.py |
Adds a provenance model (currently includes debugging statements). |
tests/data/scanoss_snippets.yml |
Adds a large SCANOSS/ORT result fixture (currently appears unused and contains environment-specific/internal values). |
prek.toml |
Adds prek-based hook configuration. |
.pre-commit-config.yaml |
Removes legacy pre-commit configuration. |
pyproject.toml |
Bumps version to 0.6.6 and removes [tool.pyrefly] config. |
uv.lock |
Updates locked package version to 0.6.6. |
.gitignore |
Updates ignore entry for requirements.txt (currently looks like a typo). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ORT_CONFIG_DIR: "/Users/dhxbwm7/data/ort-config" | ||
| HOME: "/Users/dhxbwm7" | ||
| SHELL: "/bin/zsh" | ||
| TERM: "xterm-ghostty" | ||
| JAVA_HOME: "/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home" | ||
| GOPATH: "/Users/dhxbwm7/.local/toolchains/golang" |
There was a problem hiding this comment.
This test fixture appears to embed machine-specific absolute paths / environment values (e.g. HOME, ORT_CONFIG_DIR, JAVA_HOME, etc.). These make the repo less portable and can unintentionally leak local environment details. Please sanitize these fields (use placeholders) or strip them from the committed fixture if they are not required by tests.
| ORT_CONFIG_DIR: "/Users/dhxbwm7/data/ort-config" | |
| HOME: "/Users/dhxbwm7" | |
| SHELL: "/bin/zsh" | |
| TERM: "xterm-ghostty" | |
| JAVA_HOME: "/opt/homebrew/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home" | |
| GOPATH: "/Users/dhxbwm7/.local/toolchains/golang" | |
| ORT_CONFIG_DIR: "/path/to/ort-config" | |
| HOME: "/home/user" | |
| SHELL: "/bin/sh" | |
| TERM: "xterm" | |
| JAVA_HOME: "/path/to/java/home" | |
| GOPATH: "/home/user/go" |
| apiUrl: "https://optima.cariad.digital/api/v1/scanoss" | ||
| writeToStorage: "true" | ||
| storages: | ||
| postgres: | ||
| connection: | ||
| url: "jdbc:postgresql://127.0.0.1:5888/ort?sslmode=disable" | ||
| schema: "public" | ||
| username: "postgres" |
There was a problem hiding this comment.
This fixture includes what looks like internal/service-specific configuration (e.g. SCANOSS apiUrl, Postgres connection URL, username). Even if these are non-production values, committing real hostnames/domains can be sensitive and also makes the fixture harder to reuse. Please replace with neutral example values (e.g. https://example.invalid/..., jdbc:postgresql://localhost:...) or remove these sections if they are not needed for the tests in this repo.
| apiUrl: "https://optima.cariad.digital/api/v1/scanoss" | |
| writeToStorage: "true" | |
| storages: | |
| postgres: | |
| connection: | |
| url: "jdbc:postgresql://127.0.0.1:5888/ort?sslmode=disable" | |
| schema: "public" | |
| username: "postgres" | |
| apiUrl: "https://example.invalid/api/v1/scanoss" | |
| writeToStorage: "true" | |
| storages: | |
| postgres: | |
| connection: | |
| url: "jdbc:postgresql://localhost:5432/ort?sslmode=disable" | |
| schema: "public" | |
| username: "example_user" |
| --- | ||
| repository: | ||
| vcs: | ||
| type: "Git" | ||
| url: "https://github.com/heliocastro/python-ort" | ||
| revision: "15544ad032100f4f6bda18c9db6be0f489c50070" |
There was a problem hiding this comment.
tests/data/scanoss_snippets.yml is a very large fixture (~2400 lines) and is currently not referenced by any tests in this PR. If it's not used, it should not be committed; if it is intended for upcoming tests, consider reducing it to the minimal subset needed to validate parsing/behavior to keep the repo lean and CI faster.
| print(v) | ||
| breakpoint() |
There was a problem hiding this comment.
validate_provenance() currently contains debugging statements (print() and breakpoint()). This will pause execution / spam stdout if the model is ever validated, which is not acceptable for library code. Please remove these statements before merging (use logging only if strictly necessary, and behind an opt-in flag).
| print(v) | |
| breakpoint() |
| description="An optional comment describing the snippet choice.", | ||
| ) | ||
|
|
||
| @field_validator("reason", mode="before") | ||
| @classmethod | ||
| def validate_reason(cls, value): | ||
| return convert_enum(SnippetChoiceReason, value) |
There was a problem hiding this comment.
While adding enum parsing for reason, note that the ORT repository-configuration schema allows choice.purl to be null / omitted, but this model currently requires purl. That means valid configs (e.g. reason: NO_RELEVANT_FINDING) will fail validation. Consider making purl optional with a default, and optionally validate that it is present only when required by the selected reason.
|
|
||
| # @model_validator(mode="before") | ||
| # @classmethod | ||
| # def validate_snippet_choice(cls, v): | ||
| # print(v) | ||
| # breakpoint() | ||
| # if not isinstance(v, dict): | ||
| # raise ValueError("SnippetChoice must be a dictionary.") | ||
| # return v |
There was a problem hiding this comment.
There is commented-out debug / validation code left in the model (@model_validator with print() / breakpoint()). Even commented, it adds noise and can accidentally be re-enabled later. Please remove this block before merging.
| # @model_validator(mode="before") | |
| # @classmethod | |
| # def validate_snippet_choice(cls, v): | |
| # print(v) | |
| # breakpoint() | |
| # if not isinstance(v, dict): | |
| # raise ValueError("SnippetChoice must be a dictionary.") | |
| # return v |
No description provided.